Skip to content

feat: Implement custom stickiness #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Dec 14, 2021

Conversation

sighphyre
Copy link
Member

  • Add custom stickiness for feature variants
  • Add custom stickiness for flexible rollout strategies
  • Implement custom stickiness property on variant defs
  • Force context to resolve to nil instead of crashing with empty props

@ivarconr ivarconr requested a review from rarruda December 1, 2021 11:28
@ivarconr
Copy link
Member

ivarconr commented Dec 1, 2021

@sighphyre looks like integration tests for variants is currently failing
(https://github.com/Unleash/client-specification/blob/master/specifications/08-variants.json)

also, it needs to be updated to latest version, with custom stickiness examples:
https://github.com/Unleash/unleash-client-ruby/blob/master/.github/workflows/pull_request.yml#L34

@coveralls
Copy link

coveralls commented Dec 2, 2021

Pull Request Test Coverage Report for Build 1574701104

  • 33 of 34 (97.06%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+5.2%) to 95.442%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/unleash/variant_definition.rb 3 4 75.0%
Files with Coverage Reduction New Missed Lines %
lib/unleash/strategy/flexible_rollout.rb 1 80.0%
Totals Coverage Status
Change from base Build 1572398961: 5.2%
Covered Lines: 1424
Relevant Lines: 1492

💛 - Coveralls

@sighphyre sighphyre marked this pull request as ready for review December 2, 2021 09:18
@rarruda rarruda added this to the 4.x milestone Dec 2, 2021
@rarruda
Copy link
Collaborator

rarruda commented Dec 2, 2021

Will review this as soon as i get a chance. This PR should implement #41 and #42 ! 🚀

Copy link
Collaborator

@rarruda rarruda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to have #71 merged (and this PR rebased on top of it) before approving it, as it does have a change which contains side-effects.

@sighphyre sighphyre force-pushed the feature/implement_custom_stickiness branch from ecb222d to 1d97b20 Compare December 3, 2021 08:10
@sighphyre sighphyre requested a review from rarruda December 3, 2021 08:58
Copy link
Collaborator

@rarruda rarruda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! But I do have a couple of concerns that I would like to see addressed before merging:

  • change the code for resolving stickiness to something more ruby idiomatic
  • not changing the signature (results) for get_by_name() in this PR.
  • rebase this branch to master, and run bundle exec rubocop to catch any code style deviations from the project standard.

@sighphyre sighphyre force-pushed the feature/implement_custom_stickiness branch from 040d7da to 540b143 Compare December 6, 2021 07:19
Simon Hornby and others added 9 commits December 10, 2021 14:06
- Add custom stickiness for feature variants
- Add custom stickiness for flexible rollout strategies
- Implement custom stickiness property on variant defs
- Force context to resolve to nil instead of crashing with empty props
- Upgrade CI to use latest client spec
- Fix an issued where custom properties would resolve incorrectly if camel cased in context
- Fix an issue where stickiness could be nil instead of 'default'
Improve readability of stickiness resolution

Co-authored-by: Renato Arruda <[email protected]>
@sighphyre sighphyre force-pushed the feature/implement_custom_stickiness branch from 4a7e739 to 3abfee7 Compare December 10, 2021 12:25
Copy link
Collaborator

@rarruda rarruda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I can't spot anything strange anymore :)

@sighphyre sighphyre merged commit 7f6d66f into master Dec 14, 2021
@rarruda rarruda deleted the feature/implement_custom_stickiness branch December 14, 2021 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants